Repository navigation
Conversation
| // release, before an upgrade or a rollback. Write does not fsync, so | ||
| // sync this one before deleting that copy, or a node crash could lose | ||
| // both. Only the first save after an upgrade or a rollback finds one. | ||
| stale := otherSnapshotStateFiles(a.actorUID) |
There was a problem hiding this comment.
🤖 Instead of lazily reading/migrating/deleting across both locations at runtime (and calling os.Stat on the other directory on every save), could we migrate any state files from the other location (snapshots/<uid>/snapshot-state.json -> actors/<uid>/snapshot-state.json here, and the reverse in #3732) once during atelet startup before snapshots.Start() / serving RPCs?
Since only one atelet runs on a node, a file at the other location can only have been written before atelet started:
- Migrating at startup means no state mutations ever happen while two copies exist (both files can only exist simultaneously if
ateletcrashed mid-migration betweenWriteSyncandos.Remove, when their contents are identical). - Runtime reads (
readSnapshotState), saves (writeStateLocked), deletes (removeSnapshotState),startup discovery (savedActorUIDs), and directory cleanup (actordirs) can all remain single-location (ActorSnapshotStateFile), avoiding anos.Staton the other disk on every save.
There was a problem hiding this comment.
Done. The move now happens once at atelet startup, before the tracker is created.
8acbbf1 to
f0da3f2
Compare
anishgang
changed the title
| if err := os.Remove(from); err != nil { | ||
| return fmt.Errorf("while removing %s: %w", from, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🤖 Nit: After removing from (snapshots/<uid>/snapshot-state.json), should we also try to remove filepath.Dir(from) if it is now empty (ignoring os.ErrNotExist, syscall.ENOTEMPTY, and syscall.EEXIST, like removeDetachedActorState does)?
Otherwise, rolling back from N+1 leaves an empty snapshots/<uid> directory behind when:
- The actor had already detached in N+1 (so
snapshots/<uid>held onlysnapshot-state.json), and when the detach report is later acknowledged in N,removeDetachedActorStateonly removesActorPath(actorUID). ActorPath(actorUID)is already gone (TestMigrateSnapshotStateFilesRemovesFileOfGoneActor), leaving nothing to ever clean up the emptysnapshots/<uid>directory.- The actor was started (
BeginStart) in N+1 before taking any checkpoint.
There was a problem hiding this comment.
Done. Also, #3732 doesn't do this, since there the source directory is the actor's own directory, and removing it would drop the actor's state.
| // releases (see docs/reference/version-skew-policy.md). atelet moves a file it | ||
| // finds here back to ActorSnapshotStateFile at startup, so a rollback from the | ||
| // next release keeps the actor's snapshots. | ||
| func OtherSnapshotStateFile(actorUID string) string { |
There was a problem hiding this comment.
Can you call this "NextSnapshtStateFile"
| } | ||
|
|
||
| // MigrateSnapshotStateFiles moves each actor's snapshot state file from | ||
| // ateletpath.OtherSnapshotStateFile, where another release keeps it, to |
There was a problem hiding this comment.
instead of "another release", can you describe it as "future release"
f0da3f2 to
f16a385
Compare
…startup This is the first of two releases that move each actor's snapshot-state.json from /var/lib/ate/actors/<uid>/ to /var/lib/ate/snapshots/<uid>/, so its writes stop waiting on the boot disk. The version skew policy needs a node layout change to span two minor releases. This release still keeps the file in the old place. At startup, before the snapshot tracker is created, atelet moves any file the next release left in the snapshot directory back to the old place, so a rollback from that release keeps every actor's snapshots. Only one atelet runs on a node, so such a file was written before startup, and the rest of atelet keeps using one place. Each moved file is synced before the original is removed, so a crash leaves two identical copies that the next startup finishes moving. A file whose actor directory is gone is removed instead. atelet exits if a move fails, so it never loses track of an actor's snapshots. Co-authored-by: Michelle Au <msau@google.com> Signed-off-by: Anish Gangu <anishgang@google.com>
f16a385 to
d01066f
Compare
Michelle Au (msau42)
left a comment
Michelle Au (msau42)
left a comment
There was a problem hiding this comment.
lgtm, we need to wait until 11/18 for the branch to reopen
This is the first of two releases that move each actor's snapshot-state.json from /var/lib/ate/actors// to /var/lib/ate/snapshots//, so its writes stop waiting on the boot disk.
The version skew policy needs a node layout change to span two minor releases. This release still keeps the file in the old place. At startup, before the snapshot tracker is created, atelet moves any file the next release left in the snapshot directory back to the old place, so a rollback from that release keeps every actor's snapshots. Each moved file is synced before the original is removed, and atelet exits if a move fails.
The follow up PR that writes the new place is #3732, and will merge in the next minor release.
AI use: Ran CL review